feat: clearing library#357
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughA new clearing queue library has been added, including a CosmWasm contract, message and state definitions, configuration, testing suite, and documentation. The contract manages withdrawal obligations in a FIFO queue, validating inputs, maintaining state, and executing settlements from a designated input account. Changes
Sequence Diagram(s)sequenceDiagram
participant Strategist
participant ClearingQueueContract
participant SettlementAccount
participant Recipient
Strategist->>ClearingQueueContract: RegisterObligation(recipient, payout, id)
ClearingQueueContract->>ClearingQueueContract: Validate & enqueue obligation
Strategist->>ClearingQueueContract: SettleNextObligation()
ClearingQueueContract->>ClearingQueueContract: Pop oldest obligation
ClearingQueueContract->>SettlementAccount: Check balance for payout
alt Sufficient funds
ClearingQueueContract->>SettlementAccount: BankSend(payout to recipient)
SettlementAccount->>Recipient: Transfer coins
else Insufficient funds
ClearingQueueContract->>Strategist: Error (insufficient funds)
end
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
Cargo.toml (1)
93-93: Maintain logical ordering of workspace dependencies.The new
valence-clearing-queueentry should be grouped consistently with othercontracts/libraries/*dependencies—ideally in alphabetical order. Consider moving it beforevalence-supervaults-withdrawerto keep the list organized.Apply this diff to reorder:
- valence-supervaults-withdrawer = { path = "contracts/libraries/supervaults-withdrawer", features = ["library"] } - valence-clearing-queue = { path = "contracts/libraries/clearing-queue", features = ["library"] } + valence-clearing-queue = { path = "contracts/libraries/clearing-queue", features = ["library"] } + valence-supervaults-withdrawer = { path = "contracts/libraries/supervaults-withdrawer", features = ["library"] }contracts/libraries/clearing-queue/src/state.rs (1)
27-27: Fix typo in field name.The field name
enque_blockshould beenqueue_blockfor correct English spelling.- pub enque_block: BlockInfo, + pub enqueue_block: BlockInfo,docs/src/libraries/cosmwasm/clearing_queue.md (1)
6-6: Apply style improvements for better readability.The static analysis tools have identified valid style improvements that would make the documentation more concise.
-> This library functions solely as a settlement engine. The settlement account funding (liquidity-management) flow is outside of its scope and is managed by a strategist. This management process likely involves monitoring both the settlement account balance and the obligation queue in order to ensure the settlement account maintains sufficient liquidity for obligation settlements. +> This library functions solely as a settlement engine. The settlement account funding (liquidity-management) flow is outside its scope and is managed by a strategist. This management process likely involves monitoring both the settlement account balance and the obligation queue to ensure the settlement account maintains sufficient liquidity for obligation settlements.🧰 Tools
🪛 LanguageTool
[style] ~6-~6: This phrase is redundant. Consider using “outside”.
Context: ... funding (liquidity-management) flow is outside of its scope and is managed by a strategis...(OUTSIDE_OF)
[style] ~6-~6: Consider a more concise word here.
Context: ...ccount balance and the obligation queue in order to ensure the settlement account maintains...(IN_ORDER_TO_PREMIUM)
contracts/libraries/clearing-queue/src/testing/builder.rs (1)
79-84: Remove debug print statementThe
println!statement should be removed from test code. Use proper logging if needed.pub fn with_input_balances(mut self, input_coins: Vec<Coin>) -> Self { self.input_balances = input_coins; - println!("input balances = {:?}", self.input_balances); self }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (14)
Cargo.toml(1 hunks)contracts/libraries/clearing-queue/.cargo/config.toml(1 hunks)contracts/libraries/clearing-queue/Cargo.toml(1 hunks)contracts/libraries/clearing-queue/README.md(1 hunks)contracts/libraries/clearing-queue/src/contract.rs(1 hunks)contracts/libraries/clearing-queue/src/lib.rs(1 hunks)contracts/libraries/clearing-queue/src/msg.rs(1 hunks)contracts/libraries/clearing-queue/src/state.rs(1 hunks)contracts/libraries/clearing-queue/src/testing/builder.rs(1 hunks)contracts/libraries/clearing-queue/src/testing/mod.rs(1 hunks)contracts/libraries/clearing-queue/src/testing/suite.rs(1 hunks)contracts/libraries/clearing-queue/src/testing/tests.rs(1 hunks)docs/src/SUMMARY.md(1 hunks)docs/src/libraries/cosmwasm/clearing_queue.md(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/src/libraries/cosmwasm/clearing_queue.md
[style] ~6-~6: This phrase is redundant. Consider using “outside”.
Context: ... funding (liquidity-management) flow is outside of its scope and is managed by a strategis...
(OUTSIDE_OF)
[style] ~6-~6: Consider a more concise word here.
Context: ...ccount balance and the obligation queue in order to ensure the settlement account maintains...
(IN_ORDER_TO_PREMIUM)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build-and-deploy
- GitHub Check: setup-local-ic
🔇 Additional comments (39)
docs/src/SUMMARY.md (1)
32-32:Details
✅ Verification successful
Verify the documentation file path.
Ensure that the
Clearing Queueentry at line 32 points to an existing file and avoids broken links:
- The path should be
docs/src/libraries/cosmwasm/clearing_queue.md.- Confirm the file name matches exactly (including underscores).
🏁 Script executed:
#!/bin/bash # Verify that the linked documentation file exists if [ ! -f docs/src/libraries/cosmwasm/clearing_queue.md ]; then echo "Error: docs/src/libraries/cosmwasm/clearing_queue.md not found" exit 1 fiLength of output: 62
Confirmed: Clearing Queue documentation link is valid
The file
docs/src/libraries/cosmwasm/clearing_queue.mdexists and matches the link indocs/src/SUMMARY.md, so the path is correct.contracts/libraries/clearing-queue/.cargo/config.toml (1)
1-3: Approve Cargo aliases for build and schema.The
[alias]section defines convenient shortcuts for building the WASM binary and generating the schema, consistent with other libraries in the workspace.contracts/libraries/clearing-queue/src/testing/mod.rs (1)
1-3: Testing module structure looks correct.The
builderandsuitemodules are public for test utilities, and the privatetestsmodule groups test cases. All are scoped undertestinginlib.rswith#[cfg(test)], ensuring they are only compiled during tests.contracts/libraries/clearing-queue/src/lib.rs (2)
1-4: Public API modules defined appropriately.The
contract,msg, andstatemodules expose the core clearing queue logic, message types, and state management. No visibility or import issues detected here.
5-6: Test harness inclusion is correct.The
#[cfg(test)] mod testing;ensures that all test utilities and cases intesting/are only compiled for test builds. This aligns with Rust best practices.contracts/libraries/clearing-queue/Cargo.toml (1)
1-31: LGTM! Well-structured package configuration.The Cargo.toml follows CosmWasm best practices with proper workspace inheritance, appropriate crate types for both library and binary usage, and a sensible "library" feature flag for conditional compilation. The dependencies are well-chosen for a CosmWasm smart contract library.
contracts/libraries/clearing-queue/README.md (1)
1-37: Excellent documentation with clear flow visualization.The README provides comprehensive coverage of the clearing queue library's purpose and functionality. The mermaid diagram effectively illustrates the interaction flow between components, and the configuration section clearly explains the settlement input account concept.
contracts/libraries/clearing-queue/src/state.rs (1)
1-16: Good state design for FIFO queue management.The state structures are well-designed for managing a clearing queue system. Using a separate map for tracking registered obligation IDs effectively prevents duplicate processing, and the QueueMap provides proper FIFO semantics.
docs/src/libraries/cosmwasm/clearing_queue.md (1)
1-47: Comprehensive documentation with minor style improvements needed.The documentation provides excellent coverage of the clearing queue library's functionality, including a helpful function table and clear configuration examples. The separation of concerns between settlement and liquidity management is well-explained.
🧰 Tools
🪛 LanguageTool
[style] ~6-~6: This phrase is redundant. Consider using “outside”.
Context: ... funding (liquidity-management) flow is outside of its scope and is managed by a strategis...(OUTSIDE_OF)
[style] ~6-~6: Consider a more concise word here.
Context: ...ccount balance and the obligation queue in order to ensure the settlement account maintains...(IN_ORDER_TO_PREMIUM)
contracts/libraries/clearing-queue/src/testing/builder.rs (4)
1-15: LGTM!The imports and test user constants are appropriate for the testing environment.
16-48: LGTM!The builder struct and trait implementation follow the delegation pattern correctly.
86-89: LGTM!Using
Addr::uncheckedis acceptable in test code where we control the inputs.
91-129: LGTM!The build method correctly sets up the test environment with proper initialization logic.
contracts/libraries/clearing-queue/src/contract.rs (6)
1-15: LGTM!Standard CosmWasm imports and proper use of environment variables for contract metadata.
16-41: LGTM!Entry points correctly delegate to the base library implementation.
43-57: LGTM!Config update correctly delegates to the configuration update logic.
86-137: LGTM!Comprehensive validation and proper storage of withdrawal obligations. Good use of
ensure!macros for validation.
139-182: LGTM!Settlement logic correctly validates balances and executes transfers with proper authorization.
184-236: LGTM!Query implementations are correct with proper pagination support for obligations.
contracts/libraries/clearing-queue/src/testing/suite.rs (3)
1-15: LGTM!Test imports and denomination constants are appropriate.
26-80: LGTM!Test helper methods provide clean abstractions for contract interactions.
82-126: LGTM!Query methods and trait implementation correctly delegate to the inner test suite.
contracts/libraries/clearing-queue/src/testing/tests.rs (10)
1-12: LGTM! Well-organized imports and constants.The imports are appropriate for CosmWasm testing, and the
INVALID_ADDRconstant provides a reusable invalid address for validation tests.
14-34: LGTM! Comprehensive address validation testing.These tests properly verify that invalid addresses are rejected during both instantiation and configuration updates, with appropriate error message validation.
36-62: LGTM! Essential settlement precondition validation.These tests properly verify critical settlement preconditions:
- Empty queue protection prevents invalid operations
- Insufficient funds checking prevents failed settlements with specific error messages including denomination details
64-96: LGTM! Thorough obligation registration validation.These tests comprehensively validate obligation registration requirements:
- Non-empty payout coins enforcement
- Valid recipient address validation
- Non-zero coin amount validation with specific denomination error messages
98-117: LGTM! Essential happy path validation.This test properly verifies the basic obligation registration flow, checking both queue metadata (count) and actual obligation list contents.
119-170: LGTM! Excellent FIFO queue behavior validation.This test comprehensively validates the FIFO queue mechanics:
- Verifies correct ordering when multiple obligations are registered
- Confirms that settlement processes the oldest obligation first
- Tests both queue metadata and obligation ordering before/after settlement
172-201: LGTM! Critical duplicate prevention security testing.These tests validate essential security features:
- Prevention of duplicate obligation IDs during registration
- Ensuring obligation IDs remain permanently reserved even after settlement
This prevents double-accounting attacks and maintains system integrity.
203-239: LGTM! Comprehensive multi-denomination settlement testing.This test thoroughly validates settlement with multiple coin denominations:
- Verifies correct balance deduction from input account for each denomination
- Confirms proper credit to user account for each denomination
- Tests the complete settlement flow for complex obligations
241-283: LGTM! Thorough multi-user settlement validation.This test comprehensively validates settlement across multiple users:
- Verifies correct FIFO processing for different users
- Ensures proper balance accounting for each user
- Confirms total input account balance reduction matches settled amounts
285-323: LGTM! Complete multi-denomination obligation testing.This test validates complex settlement scenarios:
- Multiple obligations for the same user with different denominations
- Independent denomination tracking and settlement
- Proper balance accounting per denomination across settlements
contracts/libraries/clearing-queue/src/msg.rs (7)
1-9: LGTM! Appropriate imports for CosmWasm library messaging.All necessary dependencies are included for schema generation, validation, error handling, and library interface functionality.
11-25: LGTM! Well-designed configuration structures.Clear separation between input configuration (
LibraryConfig) and validated configuration (Config) with appropriate types:
LibraryAccountTypefor flexible input handlingAddrfor validated addresses- Clear documentation explaining the settlement account purpose
27-40: LGTM! Well-structured execute messages for queue operations.The message design is appropriate:
RegisterObligationincludes all necessary parameters (recipient, payout, unique ID)SettleNextObligationis parameter-free, correctly implementing FIFO processing- Good documentation explaining each message's purpose
42-55: LGTM! Clean configuration API with proper validation.The implementation provides:
- Convenient constructor with flexible input type conversion
- Centralized validation logic in
do_validate()- Proper error handling returning
LibraryError
57-68: LGTM! Proper validation trait implementation.Correctly implements the library validation pattern:
- Conditional compilation for non-wasm
pre_validate()- Reuses centralized validation logic
- Returns validated
Configwith proper error handling
70-82: LGTM! Proper configuration update handling.The implementation correctly handles optional configuration updates:
- Loads existing configuration from storage
- Updates only specified fields (using
Optionfor selective updates)- Proper error handling and storage operations
Note:
LibraryConfigUpdateappears to be generated by theValenceLibraryInterfacemacro onLibraryConfig.
84-115: LGTM! Comprehensive query interface for queue operations.Well-designed query system:
QueueInfoprovides essential metadata (count, pagination indices)Obligationsquery supports range-based retrieval for scalability- Response structures include all necessary fields
- Proper use of query macros for schema generation
keyleu
left a comment
keyleu
left a comment
There was a problem hiding this comment.
Current implementation works, left some suggestions for state refactor that would allow more queries to know what's going on and what happened.
| /// end index | ||
| to: Option<u64>, | ||
| }, | ||
| } |
There was a problem hiding this comment.
I'd add a query to see if a withdrawal_request is already registered. I see this being useful to check if we need to get a proof for a specific ID.
Or maybe a query that returns a withdrawal_request for an ID and returns None if it doesn't exist or something like that
There was a problem hiding this comment.
my thinking was that this could be done on frontend by filtering the ObligationsResponse as strategist shouldn't be concerned with individual requests
There was a problem hiding this comment.
I'm just thinking of the logic the strategist will use to request a proof. How can he know the ID that he needs to request a proof for? I guess he could fetch the current withdraw request on Ethereum and start going down from there and check if they are registered until he finds one that isnt?
There was a problem hiding this comment.
I'm not sure if strategist should be touching neutron for proof generation. feels a bit fragile given the latency of the coprocessor?
There was a problem hiding this comment.
Let's say the clearing queue is empty. And the strategist restarts. How does it know which ID to use to start asking for proofs?
There was a problem hiding this comment.
yeah, I think this problem applies not just in situations where strategist restarts but also in cases where withdraw obligations get posted out of order.
tbh I don't have a good answer because I don't know the internals of the coprocessor. hence the decoupled design of this contract - I assumed that coprocessor + strategist + ethereum state is sufficient to shield the settlement flow from the way those requests originate (other than the withdraw obligation id checks).
cc @hxrts
There was a problem hiding this comment.
Coprocessor has no state. Will only proof that a certain ID exists and give the necessary information to this contract.
We can make the strategist post the obligations always in order, but for that it needs to at least know the last one posted, currently we have no way of knowing the last posted because the clearing queue might be empty and we have no access to the other map via a query
| /// set of registered obligation ids meant to prevent double filling | ||
| /// of any given obligation | ||
| pub const REGISTERED_OBLIGATION_IDS: Map<u64, Empty> = Map::new("reg_obl_id"); |
There was a problem hiding this comment.
So currently we can only know what withdrawal obligations are currently being processed (currently in the queue), and these are currently not queriable by address.
I suggest changing this REGISTERED_OBLIGATIONS_IDS into an IndexedMap with a MultiIndex (request_addr, which would need to be added to WithdrawalObligation, along with the status, which would need to be updated after being processed).
This way we can query:
- Withdrawal obligation for a specific ID.
- Withdrawal obligations per request address. For this query we would get all withdrawal obligations that an eth address requested along with their current status.
This would be enough in case the front end would like to display this information for a user. Otherwise we don't really know much of what happened to withdrawal obligations because once they are processed they are just gone
LMK what you think.
We can of course rely only on indexing transactions on ethereum but since storing this on Neutron is so cheap anyways I don't see why not
There was a problem hiding this comment.
this kind of follows from my request above.
happy to jam on this if you feel like it's worthwhile, but in the current design request_addr would be purely informational and useful only for frontend as it has no impact on the settlements
There was a problem hiding this comment.
yeah, let's talk about this. The minimum we need here is a query on this Map here for us to know if something is pending to be registered or not. Otherwise we can't know if something hasn't been registered yet or is completed. That's the bare minimum, rest would just be extras to make it easier
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
docs/src/libraries/cosmwasm/clearing_queue.md (2)
5-7: Refine wording for clarity and conciseness.Reduce redundancy by replacing "outside of its scope" with "outside its scope" and "in order to ensure" with "to ensure".
- > This library functions solely as a settlement engine. The settlement account funding (liquidity-management) flow is outside of its scope and is managed by a strategist. This management process likely involves monitoring both the settlement account balance and the obligation queue in order to ensure the settlement account maintains sufficient liquidity for obligation settlements. + > This library functions solely as a settlement engine. The settlement account funding (liquidity-management) flow is outside its scope and is managed by a strategist. This management process likely involves monitoring both the settlement account balance and the obligation queue to ensure the settlement account maintains sufficient liquidity for obligation settlements.🧰 Tools
🪛 LanguageTool
[style] ~6-~6: This phrase is redundant. Consider using “outside”.
Context: ... funding (liquidity-management) flow is outside of its scope and is managed by a strategis...(OUTSIDE_OF)
[style] ~6-~6: Consider a more concise word here.
Context: ...ccount balance and the obligation queue in order to ensure the settlement account maintains...(IN_ORDER_TO_PREMIUM)
29-33: Consider adding a usage example.Providing a Rust code snippet that demonstrates instantiating the library, registering an obligation, and settling the next obligation would help users quickly understand the API and integration points.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
contracts/libraries/clearing-queue/README.md(1 hunks)contracts/libraries/clearing-queue/src/contract.rs(1 hunks)contracts/libraries/clearing-queue/src/msg.rs(1 hunks)contracts/libraries/clearing-queue/src/testing/suite.rs(1 hunks)contracts/libraries/clearing-queue/src/testing/tests.rs(1 hunks)docs/src/libraries/cosmwasm/clearing_queue.md(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/libraries/clearing-queue/src/msg.rs
🧰 Additional context used
🪛 LanguageTool
docs/src/libraries/cosmwasm/clearing_queue.md
[style] ~6-~6: This phrase is redundant. Consider using “outside”.
Context: ... funding (liquidity-management) flow is outside of its scope and is managed by a strategis...
(OUTSIDE_OF)
[style] ~6-~6: Consider a more concise word here.
Context: ...ccount balance and the obligation queue in order to ensure the settlement account maintains...
(IN_ORDER_TO_PREMIUM)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: setup-local-ic
- GitHub Check: build-and-deploy
🔇 Additional comments (11)
contracts/libraries/clearing-queue/README.md (1)
1-37: Excellent documentation with clear system overview.The README provides a comprehensive overview of the clearing queue library with:
- Clear explanation of FIFO queue semantics for withdrawal obligations
- Well-designed mermaid diagram illustrating the system flow
- Proper documentation of the configuration structure
- Clear explanation of settlement account validation
The documentation aligns well with the implementation and provides good guidance for users.
contracts/libraries/clearing-queue/src/contract.rs (4)
16-24: Well-structured instantiate function.The instantiate function properly delegates to the library base, maintaining consistency with the framework architecture.
86-137: Comprehensive obligation registration validation.The registration function implements robust validation:
- Prevents duplicate obligation IDs (lines 95-100)
- Validates non-empty payout coins (lines 103-108)
- Ensures non-zero coin amounts (lines 111-119)
- Validates recipient address format (line 122)
- Maintains both queue and ID tracking for consistency
The error messages are clear and informative for debugging.
139-181: Secure settlement implementation with proper validation.The settlement function ensures safety through:
- FIFO queue processing (line 141)
- Empty queue error handling (lines 143-150)
- Settlement account balance validation for each coin (lines 157-167)
- Proper fund transfer execution using
execute_on_behalf_of(line 178)This prevents failed transactions due to insufficient funds and maintains queue integrity.
211-234: Complete query interface implementation.The query functions provide comprehensive access to:
- Contract ownership and processor information
- Library configuration (both validated and raw)
- Queue state and obligations with range support
This enables proper monitoring and debugging capabilities.
contracts/libraries/clearing-queue/src/testing/suite.rs (2)
16-80: Well-designed testing suite with comprehensive functionality.The testing suite provides excellent coverage with:
- Clear methods for registering obligations and settlement operations
- Proper configuration update functionality
- Comprehensive query methods for balance and queue state validation
- Clean error handling using
AnyResultThe method signatures are intuitive and support thorough integration testing.
102-126: Proper trait implementation for framework integration.The
LibraryTestSuitetrait implementation correctly delegates to the inner suite, maintaining consistency with the testing framework while exposing necessary functionality for clearing queue testing.contracts/libraries/clearing-queue/src/testing/tests.rs (4)
14-96: Excellent input validation test coverage.The validation tests comprehensively cover all input validation scenarios:
- Invalid addresses during instantiation and updates (lines 14-34)
- Empty queue settlement attempts (lines 36-42)
- Insufficient settlement account balance (lines 44-62)
- Empty payout coins and invalid recipient addresses (lines 64-86)
- Zero amount validation (lines 88-96)
Each test uses precise error message matching for accurate validation.
119-170: Thorough FIFO queue behavior validation.This test excellently verifies the First-In-First-Out queue semantics:
- Registers obligations in a specific order (user_1 → user_3 → user_2)
- Validates queue state before and after operations
- Confirms that settlement processes the oldest obligation first
- Verifies remaining queue order after settlement
This is critical for ensuring the queue operates correctly under various scenarios.
172-201: Critical duplicate ID prevention testing.These tests ensure the important security feature of preventing duplicate obligation processing:
- Tests immediate duplicate registration (lines 172-183)
- Tests duplicate registration after settlement (lines 185-201)
- Validates that obligation IDs remain blocked even after fulfillment
This prevents double-spending and maintains system integrity.
203-323: Comprehensive balance and settlement verification.The multi-denomination and multi-user tests provide excellent coverage:
- Validates balance updates for multiple denominations (lines 203-239)
- Tests multiple user settlements with proper balance tracking (lines 241-283)
- Verifies cross-denomination settlement accuracy (lines 285-323)
These tests ensure the settlement mechanism works correctly across complex scenarios and properly updates all account balances.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
contracts/libraries/clearing-queue/src/contract.rs (2)
139-183: Consider potential race conditions in balance validation.The settlement logic is well-structured, but there's a potential race condition between balance checking and execution:
Race condition: The balance is checked for each coin (lines 157-168), but the actual transfer happens later via
execute_on_behalf_of. If other transactions modify the settlement account balance between the check and execution, the transfer could fail.Error handling: The current implementation will fail the entire settlement if any coin has insufficient balance, which is appropriate.
Consider one of these approaches to handle the race condition:
- Accept the risk: Document that settlement can fail if balance changes between check and execution
- Atomic validation: Move balance checking to the settlement account's logic if possible
- Retry mechanism: Implement a retry mechanism for failed settlements
The current implementation is acceptable for most use cases, but the race condition should be documented.
69-84: Suggest adding queue size limits for DoS protection.The function routing is clean, but consider adding queue size limits to prevent potential DoS attacks through unlimited obligation registration.
Consider adding a maximum queue size check in the registration function:
fn try_register_withdraw_obligation( deps: DepsMut, env: Env, - _cfg: Config, + cfg: Config, recipient: String, payout_coins: Vec<Coin>, id: Uint64, ) -> Result<Response, LibraryError> { + // Check queue size limit (if configured) + if let Some(max_size) = cfg.max_queue_size { + let current_size = CLEARING_QUEUE.len(deps.storage)?; + ensure!( + current_size < max_size, + LibraryError::ExecutionError("queue size limit exceeded".to_string()) + ); + }This would require adding
max_queue_size: Option<u64>to the Config struct.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
contracts/libraries/clearing-queue/src/contract.rs(1 hunks)contracts/libraries/clearing-queue/src/msg.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/libraries/clearing-queue/src/msg.rs
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: solidity contracts
- GitHub Check: setup-local-ic
- GitHub Check: build-and-deploy
🔇 Additional comments (5)
contracts/libraries/clearing-queue/src/contract.rs (5)
1-15: LGTM! Standard CosmWasm contract setup.The imports and constants follow standard CosmWasm patterns. Using environment variables for contract name and version is a good practice for build-time configuration.
16-24: LGTM! Clean instantiate function.The instantiate function properly delegates to the valence_library_base, which handles standard initialization including version info and configuration setup.
26-41: LGTM! Proper execute function delegation.The execute function correctly routes to the base library with custom handlers for function processing and config updates.
193-208: LGTM! Clean query implementation.The query functions are well-implemented with proper error handling and use standard CosmWasm query patterns.
211-234: LGTM! Comprehensive query routing.The query entry point properly handles all query types including standard valence library queries and custom clearing queue queries.
…lue; add obligation id status query
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
contracts/libraries/clearing-queue/src/msg.rs (2)
91-93: Consider past feedback about QueueInfo query design.From previous review comments, there was feedback about the
QueueInfoquery returning a struct with just a length field instead of returning a simpleu64. While the current design provides extensibility for future fields, consider if the simpler approach would be more appropriate.
106-108: ObligationStatus query addresses past concerns about ID tracking.The
ObligationStatusquery provides the ability to check if a specific obligation is settled, which partially addresses past review concerns about knowing which IDs need proof generation. However, the broader concern about strategist restart scenarios and out-of-order obligation posting may still need consideration at the architecture level.
🧹 Nitpick comments (1)
contracts/libraries/clearing-queue/src/msg.rs (1)
36-36: Consider using a more specific type for obligation ID.Using
Uint64for the obligation ID is functional, but consider if a more domain-specific type or newtype wrapper would provide better type safety and clarity about the ID's purpose and constraints.- id: Uint64, + id: u64,This would be more idiomatic Rust and align with the query interface that uses
u64.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
contracts/libraries/clearing-queue/src/contract.rs(1 hunks)contracts/libraries/clearing-queue/src/msg.rs(1 hunks)contracts/libraries/clearing-queue/src/state.rs(1 hunks)contracts/libraries/clearing-queue/src/testing/suite.rs(1 hunks)contracts/libraries/clearing-queue/src/testing/tests.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- contracts/libraries/clearing-queue/src/state.rs
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build
- GitHub Check: setup-local-ic
- GitHub Check: build-and-deploy
🔇 Additional comments (8)
contracts/libraries/clearing-queue/src/testing/tests.rs (1)
1-341: Excellent comprehensive test coverage with well-structured test cases.The test suite demonstrates thorough coverage of the clearing queue functionality including:
- Input validation (addresses, payout coins, amounts)
- Queue behavior verification (FIFO ordering)
- Settlement mechanics (balance checks, fund transfers)
- Error conditions (empty queue, insufficient funds, duplicate registrations)
- Multi-user and multi-denomination scenarios
The tests are well-organized with clear naming conventions and appropriate use of
#[should_panic]for error conditions. The builder pattern usage makes test setup clean and maintainable.contracts/libraries/clearing-queue/src/contract.rs (4)
101-106: Proper validation prevents double registration.The check to ensure obligation IDs are not already registered is essential for preventing double settlement. The error message is clear and informative.
117-125: Thorough validation of payout coins.The validation ensures each coin has a non-zero amount with clear error messages including the problematic denomination. This prevents invalid obligations from being registered.
165-181: Robust balance validation before settlement.The settlement logic properly validates that the settlement account has sufficient balance for each denomination before attempting transfer. This prevents partial settlements and provides clear error messages when funds are insufficient.
183-196: Secure fund transfer implementation.The settlement logic correctly:
- Constructs bank send messages with validated amounts
- Uses
execute_on_behalf_ofto transfer from settlement account- Updates obligation status to prevent double settlement
- Returns appropriate response
contracts/libraries/clearing-queue/src/testing/suite.rs (3)
26-48: Clean obligation registration test helper.The method properly constructs the execute message and handles the conversion from
u64toUint64for the ID parameter. The error propagation is appropriate for test scenarios.
82-107: Comprehensive query helper methods.The query methods provide good coverage for testing different aspects of the contract state:
- Account balances for settlement verification
- Queue information for length checking
- Obligation status for settlement verification
- Obligations list for FIFO order verification
109-133: Proper delegation to inner test suite.The
LibraryTestSuitetrait implementation correctly delegates all methods to the inner suite, maintaining the testing framework's interface while adding domain-specific functionality.
2 participants
closes #350
Description
Introduces the clearing queue library.
This library allows registering and settling obligations.
Sender validation
Any message sender validations are expected to be done on authorizations level.
Obligation registration
Obligations are registered with the following execute msg variant:
In turn, the following validations are performed:
idwas not registered beforepayout_coinsvector is not emptypayout_coinsamount is non-zerorecipientaddress is validFollowing the validations, the constructed obligation is pushed to the end of the queue,
and store the obligation
idto prevent double accounting.From here, the validation is considered valid and ready to be settled.
Obligation settlement
Obligations are settled with the following execute msg variant:
In turn, the following validations are performed:
After that, a
BankSendmessage is constructed and delegated to be executed on behalfof the settlement (input) account.
Summary by CodeRabbit
New Features
Documentation
Tests